Skip to content

Improve IN list performance in ExpressionInterpreter#11902

Merged
sopel39 merged 3 commits intotrinodb:masterfrom
starburstdata:ks/do_not_Create
Apr 14, 2022
Merged

Improve IN list performance in ExpressionInterpreter#11902
sopel39 merged 3 commits intotrinodb:masterfrom
starburstdata:ks/do_not_Create

Conversation

@sopel39
Copy link
Copy Markdown
Member

@sopel39 sopel39 commented Apr 11, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@sopel39 sopel39 requested a review from findepi April 11, 2022 14:41
@cla-bot cla-bot Bot added the cla-signed label Apr 11, 2022
@sopel39 sopel39 requested a review from wendigo April 11, 2022 14:42
@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Apr 11, 2022

Part of #5950

@sopel39 sopel39 force-pushed the ks/do_not_Create branch 3 times, most recently from c8e5e39 to bbf7d17 Compare April 11, 2022 15:07
findepi
findepi reviewed Apr 11, 2022
View reviewed changes
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated
Comment thread core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java Outdated
@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Apr 11, 2022

Failed due to: #11825

@sopel39 sopel39 requested a review from findepi April 12, 2022 10:40
@wendigo wendigo mentioned this pull request Apr 12, 2022
Closed
findepi
findepi reviewed Apr 12, 2022
View reviewed changes
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated
@findepi
Copy link
Copy Markdown
Member

findepi commented Apr 13, 2022

Does this improve io.trino.sql.planner.BenchmarkPlanner#planLargeInQuery results?

cc @wendigo @martint @kasiafi

findepi
findepi reviewed Apr 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one question

Comment thread core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated
@sopel39 sopel39 force-pushed the ks/do_not_Create branch 2 times, most recently from e0ff5e9 to 4fb87a6 Compare April 14, 2022 16:30
sopel39 added 2 commits April 14, 2022 18:32
@sopel39
Skip interpreting of literals when IN value is unbounded
5f168dc 
benchmarks
before
planLargeInQuery 2695,225ms/op

after
planLargeInQuery 2525,273ms/op
@sopel39
Do not use inListCache when value is an expression
02ea077 
before
planLargeInQuery 2525,273ms/op

after
planLargeInQuery 2260,965ms/op
@sopel39 sopel39 changed the title Do not create a new instance of InPredicate if it consists of literals Improve IN list performance in ExpressionInterpreter Apr 14, 2022
@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Apr 14, 2022

I've removed preserving of InPredicate instance for now. Will introduce it back when ExpressionInterpreter results are cached

Planner benchmark results:

before
planLargeInQuery 2706,743ms/op

Skip interpreting of literals when IN value is unbounded:
planLargeInQuery 2525,273ms/op

Do not use inListCache when value is an expression
planLargeInQuery 2260,965ms/op

without IN optimization (return node)
planLargeInQuery 1929,047ms/op

@sopel39 sopel39 merged commit ef75c65 into trinodb:master Apr 14, 2022
@github-actions github-actions Bot added this to the 378 milestone Apr 14, 2022
@sopel39 sopel39 mentioned this pull request Apr 14, 2022
Closed
@findepi findepi deleted the ks/do_not_Create branch April 14, 2022 20:22
@mosabua mosabua mentioned this pull request Apr 14, 2022
Merged
@v-jizhang v-jizhang mentioned this pull request Apr 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi left review comments

@wendigo wendigo wendigo approved these changes

Assignees

No one assigned

Labels

cla-signed

Milestone

378

Development

Successfully merging this pull request may close these issues.

3 participants

@sopel39 @findepi @wendigo